Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate Default/Kotlin LifecycleScopeProvider interfaces #275

Merged
merged 7 commits into from
Dec 13, 2018

Conversation

ZacSweers
Copy link
Collaborator

Let's look at just unifying this into the base class instead

TODO: benchmark kotlin's default method compilation effects compared to D8's for default ones, as the latter can have significant build time penalties since it requires recompilation of all implementers

If anyone in the community wants to try this out and let us know, please do!

@kageiit
Copy link

kageiit commented Oct 30, 2018

the latter can have significant build time penalties since it requires recompilation of all implementers

Not sure this is accurate. This depends on how build systems implement it. re-dexing of implementers only needs to happen if abi changes occur in D8. Not sure how the kotlin version works (I would guess the desugaring to be similar)

@ZacSweers
Copy link
Collaborator Author

ZacSweers commented Oct 30, 2018

if abi changes occur in D8

if you change an interface, isn't that an ABI change?

Yes it is

@ZacSweers ZacSweers force-pushed the z/defaultLifecycleScopeProvider branch from c3980c1 to be39cb9 Compare December 12, 2018 08:29
@ShaishavGandhi
Copy link
Collaborator

Want to update the sample recipes as well?

@ZacSweers ZacSweers merged commit c58ef85 into master Dec 13, 2018
@ZacSweers ZacSweers deleted the z/defaultLifecycleScopeProvider branch December 13, 2018 06:08
@ZacSweers
Copy link
Collaborator Author

I couldn't find any major build issue. At the end of the day, this would only be an issue if the interface of LifecycleScopeProvider changed, which would only be in new dependency updates and should cause a rebuild anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants